Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #423

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gpgn
Copy link

@gpgn gpgn commented Jul 8, 2023

TL;DR

Introduces new fields to the Secret object:

  • env_var
  • file

Allowing users to directly specify a name or mountPath for the Secret, without having to specify a full PodTemplate(name). The old mount_requirement can still be used. Example:

from flytekit import task, workflow, Secret
import flytekit
import os


@task(
    secret_requests=[
        Secret(
            group="user-info",
            key="user_secret",
            mount_requirement=Secret.MountType.FILE,
        ),
        Secret(
            group="user-info",
            key="user_secret",
            mount_requirement=Secret.MountType.ENV_VAR,
        ),
    ]
)
def old() -> None:
    context = flytekit.current_context()
    print("old")

    # mount_requirement=ENV_VAR
    print(context.secrets.get(env_name="_FSEC_USER-INFO_USER_SECRET"))

    # mount_requirement=FILE
    with open('/etc/flyte/secrets/user-info/user_secret', 'r') as infile:
        print(infile.readlines())
    return True

@task(
    secret_requests=[
        Secret(
            group="user-info",
            key="user_secret",
            env_var=Secret.MountEnvVar(name="foo"),
        ),
        Secret(
            group="user-info",
            key="user_secret",
            file=Secret.MountFile(path="/foo"),
        ),
    ]
)

def new() -> None:
    context = flytekit.current_context()
    print("new")

    # env_var=
    print(context.secrets.get(env_name="foo"))

    # file=
    with open('/foo/user_secret', 'r') as infile:
        print(infile.readlines())


@workflow
def training_workflow(hyperparameters: dict) -> None:
    """Put all of the steps together into a single workflow."""
    old()
    new()

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

fixes flyteorg/flyte#3053

Follow-up issue

NA

Linked PRs

@welcome
Copy link

welcome bot commented Jul 8, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@gpgn gpgn force-pushed the feat-add-optional-environment-variable-name-to-secret branch from bf435bf to 70573b1 Compare July 8, 2023 16:29
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #423 (9255f27) into master (b219c2a) will increase coverage by 2.55%.
The diff coverage is n/a.

❗ Current head 9255f27 differs from pull request most recent head 3e1ba6f. Consider uploading reports for the commit 3e1ba6f to get more accurate results

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   75.92%   78.48%   +2.55%     
==========================================
  Files          18       18              
  Lines        1458     1250     -208     
==========================================
- Hits         1107      981     -126     
+ Misses        294      212      -82     
  Partials       57       57              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 18 files with indirect coverage changes

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, could you resolve a merge conflict

Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I want to call out though that this change needs to be both forward and backwards compatible. That is,

  • people on newer versions of the flyte backend (like upgrade propeller/admin/etc after these PRs are merged) but old versions of flytekit should continue to work, and
  • people on existing versions of flyte backend (those who do not upgrade propeller/admin/etc with these PRs) but who do upgrade flytekit, should continue to work.

// +optional
oneof mount_target {
MountEnvVar env_var = 5;
MountFile file = 6;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the existing MountType enum was only used in the mount_requirement field, it'd be effectively deprecated as well right? Can we add the flag there as well?

Also since the existing enum had an Any concept, should we add that as well to the one of?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nm - let's ignore this comment

@wild-endeavor wild-endeavor changed the title Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #minor Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection Aug 14, 2023
MountFile file = 6;
}

// The name of the environment variable if the Secret is injected as environment variable. If ommitted, the default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The name of the environment variable if the Secret is injected as environment variable. If ommitted, the default
// The name of the environment variable if the Secret is injected as environment variable. If

}

// The name of the environment variable if the Secret is injected as environment variable. If ommitted, the default
// FLYTE_SECRETS_ENV_PREFIX prefix will be used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FLYTE_SECRETS_ENV_PREFIX prefix will be used.
// MountEnvVar is supplied with an empty string, FLYTE_SECRETS_ENV_PREFIX prefix will be used.

}

// The path where the Secret will be mounted. The execution will fail if the underlying key management system cannot
// satisfy that requirement. If not provided, the default location will depend on the key management system.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// satisfy that requirement. If not provided, the default location will depend on the key management system.
// satisfy that requirement. If provided with an empty string, the default location will depend on the key management system.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core feature] control ENV var name for injected secrets
3 participants